Skip to content

Prevent secret leaks from environment table wrapping - #3561

Open
thomhurst wants to merge 24 commits into
mainfrom
issue-3548-env-table-secrets
Open

Prevent secret leaks from environment table wrapping#3561
thomhurst wants to merge 24 commits into
mainfrom
issue-3548-env-table-secrets

Conversation

@thomhurst

@thomhurst thomhurst commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Summary

  • render environment-variable values as single-line, ellipsized Text cells so long or multiline values never become multiple physical log lines
  • mask values for common credential-bearing variable names, including authentication, connection-string, passphrase, Redis, Azure Functions storage, and NuGet endpoint credential conventions
  • honor SecretMaskingOptions.MaskValue for name-based masking, including the safe default fallback for blank configuration
  • preserve known non-secret PWD, OLDPWD, and SSH_AUTH_SOCK diagnostics

Validation

  • PipelineInitializerTests: 25 passed, covering narrow-console truncation, CR/LF normalization, sensitive-name conventions, exemptions, and configured mask behavior
  • ModularPipelines.sln Release build: 0 errors; 227 existing warnings
  • touched-file info-level formatting verification passed

Rotation/expiry of any credentials exposed by historical logs remains an external owner/security action.

Closes #3548

Follow-up review fix

  • Masks unchanged raw environment values before newline escaping or table ellipsis can transform them, preserving downstream whole-value secret masking.
  • Keeps already-obfuscated long values visible as their configured mask.
  • Regression coverage: PipelineInitializerTests (26/26 passing).
  • Core Release build: 0 errors (existing warnings only).

Render-width follow-up

  • Replaces the fixed raw-value cutoff with a conservative capacity derived from the actual Spectre console width, widest environment-variable name, table borders, separator, and cell padding.
  • Masks unchanged raw values before rendering whenever they cannot fit intact; preserves values that demonstrably fit and values already replaced by the obfuscator.
  • Regression covers the reported 32-character CONFIG value at width 40 plus the exact safe-fit boundary.
  • PipelineInitializerTests: 28/28 passing; scoped formatter clean.

Header-width follow-up

  • Includes the Name header width when calculating safe raw-value capacity, preventing Spectre padding around one-character variable names from exposing truncated raw values.
  • Regression coverage: PipelineInitializerTests (29/29 passing); scoped whitespace formatter clean.

Partial-obfuscation follow-up

  • Only an obfuscator result equal to the effective whole-value mask is rendered as safely transformed; partially redacted composite values are replaced wholesale.
  • PipelineInitializerTests: 30/30 passed; scoped whitespace formatter clean.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Prevent secret leaks from environment table wrapping

The fix correctly addresses the root cause from #3548: NoWrap + Ellipsis() on the value column keeps each env value on one physical console row (so runner-side ::add-mask:: can still match), MakeSingleLine neutralizes embedded CR/LF as a belt-and-suspenders measure, and the switch from Markup.Escape(...) strings to Text cells is a nice side benefit — Text never interprets markup, so it's safer against control-sequence injection than the old escaped-string approach. Tests cover the wrap/mask/CRLF cases well, including a dedicated narrow-width render check.

One architectural point worth considering, not a blocker:

Sensitive-name detection duplicates an existing configurable pattern instead of reusing it. HttpRequestFormatter/HttpResponseFormatter already solve the "which names are secret-ish" problem via HttpLoggingOptions.SensitiveHeaderNames — a public, IOptions-configurable, user-overridable list with a documented default. This PR introduces a second, parallel mechanism: SensitiveEnvironmentVariableNameParts is a private static readonly string[] baked into PipelineInitializer, with no way for a consuming pipeline to extend or override it.

Why that matters here specifically: the substring match (name.Contains("PWD", ...), .Contains("KEY", ...)) is intentionally broad per the issue's spec, but it also means standard, non-secret shell variables like PWD/OLDPWD are now always masked, and any consumer whose own naming convention happens to contain one of these substrings (e.g. a TURNKEY_ENDPOINT or PRIMARY_KEY_COLUMN env var) loses visibility into that value in the startup table with no opt-out. The HttpLoggingOptions precedent exists precisely to let consumers tune this kind of heuristic per-project.

Suggestion: lift SensitiveEnvironmentVariableNameParts into a small options record (or extend SecretMaskingOptions) following the HttpLoggingOptions.SensitiveHeaderNames shape — public static Default list, IOptions<T>-injected, user-overridable via PipelineBuilder.Services. That keeps the two "mask by name heuristic" implementations in the codebase consistent, and gives consumers an escape hatch for false positives without needing another PR/fork. Not required for this fix to be correct or safe to ship, but worth a follow-up given PWD will now be masked in effectively every pipeline run.

Everything else looks solid — validation section in the PR description matches what I see in the diff, and I didn't find any correctness issues with the masking/truncation logic itself.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review

No new changes since the previous review (the only commit added since 00ac5f7 is a merge of main, with no functional diff to the touched files) — so this is a verification pass rather than a fresh review.

Independently verified locally against the current HEAD (a7ccb9b):

  • dotnet build ModularPipelines.sln -c Release: 0 errors, 0 warnings
  • PipelineInitializerTests via TUnit --treenode-filter: 10/10 passed

The masking/truncation logic in CreateEnvironmentVariablesTable holds up: sensitive-name detection short-circuits before obfuscation runs, NoWrap + .Ellipsis() keeps each value on one physical console row, and MakeSingleLine escapes embedded CR/LF as a second line of defense. Switching from Markup.Escape(...) strings to Text cells is a good safety improvement too. No correctness issues found.

Outstanding architectural point from the prior review — still unaddressed, still not a blocker:

SensitiveEnvironmentVariableNameParts (PipelineInitializer.cs#L24-L32) is a hardcoded private static readonly string[] with no way for a consuming pipeline to extend or override it. This duplicates a pattern the codebase already solves correctly elsewhere:

  • SecretMaskingOptions (src/ModularPipelines/Options/SecretMaskingOptions.cs) already exists as a DI-configurable options record for exactly this kind of "how do we detect/mask secrets" behavior — consumers configure it via builder.Services.Configure<SecretMaskingOptions>(...).
  • HttpLoggingOptions.SensitiveHeaderNames follows the same shape (IReadOnlyList<string> + static Default) for the equivalent "which names are secret-ish" problem on the HTTP logging side.

Concretely, the substring match here is intentionally broad (Contains("KEY"), Contains("PWD"), etc.), so it now always masks PWD/OLDPWD and any variable whose name happens to contain one of these substrings (e.g. TURNKEY_ENDPOINT, PRIMARY_KEY_COLUMN). Every consumer of this framework loses visibility into those values in the startup table, with no opt-out short of a fork or a follow-up PR to this repo.

Suggested follow-up: fold the sensitive name-part list into SecretMaskingOptions and inject it into PipelineInitializer the same way ISecretObfuscator is already injected. That keeps the two "mask by name heuristic" mechanisms in the codebase consistent and gives consumers a real escape hatch for false positives.

Not required for this fix to be correct or safe to ship — flagging again since it's still open, for the author/maintainer to decide whether to track as a follow-up issue.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review

Reviewed the secret-masking / table-rendering changes in PipelineInitializer.CreateEnvironmentVariablesTable. The masking logic itself (name-heuristic short-circuit before obfuscation, CR/LF normalization applied after obfuscation, ordinal-ignore-case matching, Text instead of Markup.Escape to avoid re-interpretation) is sound and the new tests cover the intended behaviors well. One high-signal correctness bug found in the layout change — see inline comment.

No CLAUDE.md compliance issues found (only the root CLAUDE.md applies to these files, and the changes don't touch build/CI/module/codegen conventions it governs).

Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs Outdated
@thomhurst
thomhurst force-pushed the issue-3548-env-table-secrets branch from df0a3b6 to 2587cd8 Compare July 30, 2026 13:34
@thomhurst

Copy link
Copy Markdown
Owner Author

Addressed the unresolved narrow-terminal review finding in 2587cd81b2.

  • Removed NoWrap from the Value column so Spectre can shrink it instead of collapsing the Name column vertically.
  • Kept Text.Ellipsis() so secret-bearing values remain single-line and truncated.
  • Added a regression assertion that LONG_VALUE remains intact.

TDD: the focused test failed before the production change because the name rendered one character per line; it now passes.

Validation:

  • PipelineInitializerTests: 10/10 passed
  • targeted whitespace verification: passed
  • changed-file analyzers with EnableCiAnalyzers=true: passed
  • core Release build: 0 errors (227 existing warnings)

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 2587cd81b2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@thomhurst
thomhurst force-pushed the issue-3548-env-table-secrets branch from 2587cd8 to 6ee031e Compare July 30, 2026 13:40
@thomhurst

Copy link
Copy Markdown
Owner Author

Rebased onto current main at 8c247942fb. Patch equivalence preserved (025bc8f12a39e9da30554c3ec3df159559e3ba92 before and after). New head: 6ee031ef63.

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6ee031ef63

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

Code review

No issues found. Checked for bugs, CLAUDE.md compliance, and correctness of the secret-masking logic.

What I checked:

  • CreateEnvironmentVariablesTable / IsSensitiveEnvironmentVariableName / MakeSingleLine in PipelineInitializer.cs — verified the mask-vs-obfuscate ordering never renders a sensitive-named value before masking, that obfuscate() runs before MakeSingleLine, and that Text.Ellipsis() is a valid Spectre.Console fluent API (confirmed against upstream source) that correctly bounds the Value column without collapsing the Name column — addressing the narrow-terminal issue from the earlier review round.
  • Switching AddRow(string, string) (which requires Markup.Escape) to AddRow(new Text(...), new Text(...).Ellipsis()) is a nice simplification: Text doesn't parse markup, so it removes a class of "forgot to escape" bugs for free.
  • Confirmed LoggingConstants.SecretMask is reused consistently with the existing sensitive-header masking pattern in HttpRequestFormatter/HttpResponseFormatter.
  • Ran dotnet build ModularPipelines.sln -c Release (core solution, per this repo's CLAUDE.md guidance) — 0 errors.
  • Ran the PipelineInitializerTests project directly — all 10 tests pass, including the new narrow-console-truncation, CR/LF-normalization, and sensitive-name-masking regression tests.
  • Compared against the prior review thread: the "narrow terminal collapses Name column" finding called out earlier has been addressed in 6ee031ef63 with a passing regression test (EnvironmentVariables_DoNotWrapLongValues).

No CLAUDE.md violations, no compile/logic issues, no gaps in the masking heuristic that would defeat the PR's stated purpose.

@thomhurst
thomhurst force-pushed the issue-3548-env-table-secrets branch from 6ee031e to 9962bc3 Compare July 30, 2026 15:06

Copy link
Copy Markdown
Owner Author

Addressed the P2 PWD/OLDPWD masking finding in 9962bc32f1; SERVICE_PWD remains protected. Validation: PipelineInitializerTests 12/12, scoped formatting/analyzers clean, core Release build clean (227 existing warnings). @codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9962bc32f1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

Code review

No issues found. This is a follow-up review of the changes since the last Claude review — specifically commit 9962bc32f1 (fix(logging): preserve working directory vars).

What changed since the last review:

  • IsSensitiveEnvironmentVariableName in PipelineInitializer.cs now excludes exact-match PWD/OLDPWD from the PWD-substring heuristic, while names that merely contain PWD (e.g. SERVICE_PWD) are still masked — this correctly narrows the P2 finding from the prior review round (false-positive masking of the common PWD/OLDPWD shell variables) without weakening the original secret-masking heuristic.
  • Two new regression tests (EnvironmentVariables_DoNotMaskStandardWorkingDirectoryNames for PWD/OLDPWD, plus the existing SERVICE_PWD case in EnvironmentVariables_MaskSensitiveNamesWithoutRegisteredSecret) cover both sides of this boundary.

Validation performed:

  • Ran PipelineInitializerTests directly via dotnet run --project test/ModularPipelines.UnitTests -- --treenode-filter '/*/*/PipelineInitializerTests/*' — all 12 tests pass (0 failed), confirming the new working-directory-variable tests and prior masking/truncation/newline tests all still hold.
  • Re-verified the mask-vs-obfuscate ordering and Text.Ellipsis() usage from the earlier review round remain intact — no regressions introduced by this commit.
  • No CLAUDE.md compliance issues in the touched files (no scoped CLAUDE.md exists under src/ModularPipelines/Engine/Executors/ or test/ModularPipelines.UnitTests/Engine/Executors/; root guidance on test filtering/build scope was followed for validation).

Nothing further to flag — the PWD/OLDPWD fix is correct, narrowly scoped, and tested.

@thomhurst
thomhurst force-pushed the issue-3548-env-table-secrets branch from 9962bc3 to 5409ad9 Compare July 30, 2026 15:29

Copy link
Copy Markdown
Owner Author

Addressed authentication-bearing environment names in 5409ad9e42 after rebasing onto current main. PipelineInitializerTests pass 15/15, scoped formatting/analyzers pass, and the core Release build passes with 0 errors (227 existing warnings).

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5409ad9e42

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

Code review

Reviewed the latest commit, 5409ad9e42 ("fix(logging): mask auth environment values"), which is the only change since the last review round.

What changed: SensitiveEnvironmentVariableNameParts gains "AUTH", and IsSensitiveEnvironmentVariableName gains an SSH_AUTH_SOCK exemption alongside the existing PWD/OLDPWD ones (src/ModularPipelines/Engine/Executors/PipelineInitializer.cs). New tests cover masking DOCKER_AUTH_CONFIG/NPM_CONFIG__AUTH and not masking SSH_AUTH_SOCK.

Correctness: the exemption check runs before the substring Any(...) match via short-circuiting &&, so SSH_AUTH_SOCK is correctly excluded before AUTH would otherwise catch it. Ordinal-ignore-case comparisons are consistent with the rest of the method. Logic is sound and matches the stated intent.

Architectural point (repeating from earlier review rounds, still unaddressed — not a blocker): this commit is a good illustration of why the earlier suggestion still stands. Each round has added one more hardcoded substring to SensitiveEnvironmentVariableNameParts and, increasingly, one more hardcoded exemption to counteract a false positive it introduces (PWD/OLDPWD for the PWD substring, now SSH_AUTH_SOCK for AUTH). That's two growing, coupled, private string[]/exemption lists baked into PipelineInitializer with no consumer-facing way to add a project-specific exemption (e.g. a team using AUTHOR_EMAIL or OAUTH_CALLBACK_URL as a non-secret env var has no escape hatch short of a fork or another PR here).

The codebase already has the right shape for this in two places:

  • HttpLoggingOptions.SensitiveHeaderNames (src/ModularPipelines/Options/HttpLoggingOptions.cs) — IReadOnlyList<string> + static Default, IOptions-configurable via PipelineBuilder.Services.
  • SecretMaskingOptions (src/ModularPipelines/Options/SecretMaskingOptions.cs) — same DI-configuration pattern for secret-masking behavior generally.

Since this PR keeps discovering new substrings/exemptions through review iteration, that's a reasonable signal the heuristic has stabilized enough to lift into an options record (sensitive name-parts + exemption list) injected into PipelineInitializer, rather than continuing to patch a private array per follow-up PR. Still fine to land as-is and track as a separate follow-up issue.

No bugs or CLAUDE.md violations found in this commit's diff. Tests are well-targeted for the new behavior.

Copy link
Copy Markdown
Owner Author

Addressed the latest P1 review by masking documented REDIS_URL connection credentials, with regression coverage. @codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bf8a51b782

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

Code review

Reviewed the latest commit, bf8a51b782 ("fix(logging): mask Redis connection URLs"), the only change since the last review round (5409ad9e42).

What changed: adds a new SensitiveEnvironmentVariableNames exact-name list (src/ModularPipelines/Engine/Executors/PipelineInitializer.cs#L35-L38) containing "REDIS_URL", and IsSensitiveEnvironmentVariableName now masks a variable if its name exactly matches an entry in that list or contains one of the existing SensitiveEnvironmentVariableNameParts substrings. A regression test (REDIS_URL case) was added to EnvironmentVariables_MaskSensitiveNamesWithoutRegisteredSecret.

Correctness: the new Contains(name, StringComparer.OrdinalIgnoreCase) check is evaluated after the PWD/OLDPWD/SSH_AUTH_SOCK exemptions via short-circuiting &&, consistent with the existing logic, and doesn't interfere with the substring-based path. Ordinal-ignore-case comparison matches the rest of the method. No bugs found in this commit's diff, and no CLAUDE.md violations.

Architectural point (repeating from earlier review rounds — still unaddressed, still not a blocker, but worth escalating): this commit adds a third hardcoded, private list to this same mechanism — SensitiveEnvironmentVariableNameParts (substring match), the PWD/OLDPWD/SSH_AUTH_SOCK exemption list, and now SensitiveEnvironmentVariableNames (exact-name match) — all baked into PipelineInitializer with no consumer-facing way to extend or override any of them. Each review round so far has added one more entry to patch a newly-discovered gap (AUTH → needed an SSH_AUTH_SOCK exemption; now REDIS_URL needed a whole new exact-match list because it doesn't fit the substring shape). That's a strong signal this heuristic has outgrown a hardcoded array and should live in a proper options object, following the pattern already established in this codebase:

  • HttpLoggingOptions.SensitiveHeaderNames (src/ModularPipelines/Options/HttpLoggingOptions.cs) — IReadOnlyList<string> + static Default, IOptions-configurable via PipelineBuilder.Services.
  • SecretMaskingOptions (src/ModularPipelines/Options/SecretMaskingOptions.cs) — same DI-configuration pattern for secret-masking behavior generally.

Folding SensitiveEnvironmentVariableNameParts, the exemption list, and the new SensitiveEnvironmentVariableNames into one options record injected into PipelineInitializer (the same way ISecretObfuscator is already injected) would let consumers extend the list for project-specific env vars (e.g. a differently-named Redis/DB connection string var) without needing another PR to this repo each time a new secret-bearing convention is discovered — which is exactly the pattern this PR has been repeating for several rounds now.

Not required for this fix to be correct or safe to ship. Given this is now the third round adding to the same hardcoded heuristic, I'd suggest tracking the options-record refactor as a concrete follow-up issue rather than continuing to patch the array in-place.

Copy link
Copy Markdown
Owner Author

Expanded masking to standard, .NET, and Azure connection-string environment variable conventions with regression coverage. @codex review

@thomhurst

Copy link
Copy Markdown
Owner Author

Preserved the standard XAUTHORITY path in 1c0533b with a parameterized regression. Validation: PipelineInitializerTests 41/41; scoped whitespace clean. @codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1c0533b7a0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs Outdated
Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e6d022198a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs
@thomhurst

Copy link
Copy Markdown
Owner Author

Added delimiter-aware PASS classification in 8f02d0c: RABBITMQ_DEFAULT_PASS is masked without false-positive masking GIT_ASKPASS. Validation: PipelineInitializerTests 50/50; scoped whitespace clean. @codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8f02d0c7d4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b98098861c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5ea6007fbc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs
@thomhurst

Copy link
Copy Markdown
Owner Author

@claude review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 27fe1dbce2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs
Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs Outdated
@thomhurst

Copy link
Copy Markdown
Owner Author

@claude review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 894d12f64b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs
@thomhurst

Copy link
Copy Markdown
Owner Author

@claude review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b0f144d066

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs
Comment thread src/ModularPipelines/Engine/Executors/PipelineInitializer.cs
@thomhurst

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 44dfe526f6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

"KEY",
"PWD",
"CREDENTIAL",
"AUTH",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve AZURE_STORAGE_AUTH_MODE diagnostics

When an Azure storage pipeline sets the standard AZURE_STORAGE_AUTH_MODE variable to key or login, this undelimited AUTH match masks that non-secret mode. Fresh evidence beyond the earlier authentication-name exemptions is the repository's own definition of these allowed values in src/ModularPipelines.Azure/Options/AzStorageContainerMetadataShowOptions.Generated.cs:24; because the exact non-sensitive allowlist omits this name, trace output can no longer show which authentication mode was selected. Exempt this exact setting or make the authentication match boundary-aware.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: env-vars table hard-wraps long values, defeating GitHub's native secret masking

1 participant